-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New data generators #34
Conversation
updating mcal auto/finite test with latest version of mcal img gen
Updating working branch
Adding test notebook
gal_image = obs.drawImage(nx=defaults['stamp_size'], | ||
ny=defaults['stamp_size'], | ||
scale=defaults['scale']) | ||
noise = galsim.GaussianNoise() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be nice to give an instance of galsim.BaseDeviate()
here, with you would initialize with a seed that you take as input. That would help the reproducibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I do plan to do this next.
} | ||
|
||
disk_frac = 1 - bulge_frac | ||
smooth_disk_frac = 1 - knot_disk_frac |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some ref/explanation would be nice here as well.
disk_hlr = .7, | ||
bulge_frac = 0.4, | ||
knot_disk_frac = 0.7, | ||
n_knots = 100, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a very nice addition. Do you have a ref or a justification for those numbers ?
knotted_disk = galsim.RandomKnots(n_knots, | ||
half_light_radius=disk_hlr, | ||
flux=knot_disk_frac,) | ||
#rng=rng) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would definitely not remove the rng initialization. It is something that you really want to do.
scale=defaults['scale']) | ||
gal_image = obj.drawImage(nx=defaults['stamp_size'], | ||
ny=defaults['stamp_size'], | ||
scale=defaults['scale']) | ||
noise = galsim.GaussianNoise() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, seed.
autometacal/python/moments.py
Outdated
|
||
q1 = Q11 - Q22 | ||
q2 = 2*Q12 | ||
T = Q11 + Q22 + 2*tf.math.sqrt(tf.math.abs(Q11*Q22-Q12*Q12)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a big fan of this definition.. Having the sqrt is risky. I am pretty sure that ngmix doesn't use this definition either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had changed to the ngmix definition (T = Q11 + Q22), but since GalFlow uses this one, I switched it back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hummmmm..... it's not that GalFlow uses this one, it's just that in GalFlow we apply a shear g, which is different than e. I've proposed reverting back to T = Q11 + Q22 in #37
autometacal/python/moments.py
Outdated
q1 = Q11 - Q22 | ||
q2 = 2*Q12 | ||
T = Q11 + Q22 + 2*tf.math.sqrt(tf.math.abs(Q11*Q22-Q12*Q12)) | ||
result = tf.stack([q1/T, q2/T], axis=-1)[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having the size (T) in the output could be very nice!
Maybe we should use another definition of the size but T is fine for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's true, but we can make another PR for that, this is out of the scope of this one.
tests/test_tf_ngmix.py
Outdated
gals, _ = autometacal.datasets.galaxies.make_data(Ngals=Ngals, snr=100, | ||
gal_g1=np.random.uniform(-.7,.7,Ngals), | ||
gal_g2=np.random.uniform(-.7,.7,Ngals), | ||
scale=scale) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, you should really have a seed set here (for np.random
but also inside make_data
for the noise in GalSim. By the way, since you are using gaussian noise, you could do it with NumPy so you only have to initialize NumPy). It is very important that each time we run this test we get the exact same result. Also, it would be nice to know the SNR used here for those tests.
tests/test_tf_ngmix.py
Outdated
weights = autometacal.tf_ngmix.create_gmix([0.,0.,0.,0.,T,1.],'gauss') | ||
result_tf_ngmix = autometacal.tf_ngmix.get_moments(weights,pixels) | ||
|
||
assert_allclose(results_ngmix,result_tf_ngmix,rtol=1e-6,atol=1e-6) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on your definition of ellipticity (different from ngmix), I am very surprise that this test pass. Maybe if you use a gaussian profile both definition (chi/epsilon) are equivalent... This would need to be investigated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is missing an update from the tf_ngmix branch, I changed it by first applying e1e2_to_g1g2() from ngmix before comparison.
tests/test_tf_ngmix.py
Outdated
def test_tf_ngmix(): | ||
""" | ||
This test generates a simple galaxy and measure moments with ngmix, vs. | ||
tf_ngmix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry but those tests are important in my opinion, and they would required a bit more explanation. Like what is a "simple galaxy"? (SNR as well)
In addition to the comment I let on the code, here are some comments on the notebooks:
Some general comments:
|
'scale' : 0.2, | ||
'stamp_size' : 51, | ||
'psf_fwhm' : 0.9, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be set as a "global" variable since it is used in all of your functions
I've updated this branch to main. |
This PR should be dropped as #40 covers it. |
I am trying to generate a more realistic set of galaxy stamps to test autometacal. These methods will be available inside the datasets subpackage and eventually integrated into tensorflow-datasets api within autometacal.
I have created a notebook at notebooks/Datasets.ipynb to explore different ways to do so.
Currently, I have implementations of:
Both 2. and 3. have problems for sure, which is why I'm seeking some guidance from @aguinot also. I'm trying to improve 2. getting code from galaxy2galaxy now.
PS: I have also tried to do bulge+disk models (with some reasonable assumptions) by modifying 1., and tested them as I was doing before - the results were comparable to 1.